Walkthrough

Adds two configuration options (server_bundle_output_path, enforce_private_server_bundles) with validation and initialization wiring. Updates Utils path resolution to handle server bundles, explicit server bundle paths, and stricter handling of missing manifest entries. Introduces new helpers and aliases for public bundles path. Adds specs covering configuration defaults and validations.

Changes

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor App
  participant Utils as ReactOnRails::Utils
  participant Config as Configuration
  participant Manifest as Asset Manifest

  App->>Utils: bundle_js_file_path(bundle_name)
  Utils->>Utils: server_bundle?(bundle_name)
  alt Server bundle
    Utils->>Config: read server_bundle_output_path
    alt server_bundle_output_path set
      Utils-->>App: return explicit server path
    else not set
      Utils->>Utils: enforce_private_server_bundles?
      alt Enforcement ON
        Utils-->>App: raise MissingEntryError
      else Enforcement OFF
        Utils-->>App: fallback to public_bundles_full_path
      end
    end
  else Not a server bundle
    Utils->>Manifest: lookup bundle path
    alt Found
      Manifest-->>Utils: asset path
      Utils-->>App: return asset path
    else Missing
      Utils->>Utils: handle_missing_manifest_entry(bundle_name)
      alt Server bundle + Enforcement ON
        Utils-->>App: raise MissingEntryError
      else Otherwise
        Utils-->>App: fallback to public_bundles_full_path
      end
    end
  end

sequenceDiagram
  autonumber
  participant Config as Configuration
  participant Rails as Rails.env/paths

  Config->>Config: setup_config_values(...)
  Config->>Config: validate_enforce_private_server_bundles
  alt enforce_private_server_bundles = true
    Config->>Config: ensure server_bundle_output_path present
    Config->>Rails: check path not under public/
    alt Violates rules
      Config-->>Config: raise ArgumentError
    else OK
      Config-->>Config: continue
    end
  else Enforcement OFF
    Config-->>Config: continue
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I nibbled paths where bundles dwell,
A secret warren, private shell.
If servers stray to public light,
I thump the ground: “Not safe, not right!”
With maps in paw and configs true,
I hop the manifest—straight to you. 🐇✨

Pre-merge checks and finishing touches

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Code Review for PR #1805: Server Bundle Security Features

Overall Assessment

This PR adds important security features for protecting server-side rendering bundles from public exposure. The implementation is well-thought-out with good backwards compatibility. However, there are several areas that need attention.

✅ Strengths

• Maintains backwards compatibility with existing configurations
• Clear separation between public and private bundle paths
• Comprehensive validation of configuration options
• Good test coverage for the new configuration options

🔍 Issues & Suggestions

1. Critical: Path Traversal Security Risk

In lib/react_on_rails/utils.rb:80, the code directly uses the configured path without sanitization. If bundle_name contains path traversal sequences (e.g., ../../../etc/passwd), it could potentially access files outside the intended directory.

Recommendation: Sanitize the bundle name and validate the resolved path to ensure it stays within the configured directory boundaries.

2. Performance: Redundant Bundle Type Checking

The server_bundle? method is called multiple times in the execution path. Consider caching this check or restructuring the logic to minimize repeated checks.

3. Error Handling: Insufficient Context in Error Messages

In lib/react_on_rails/utils.rb:302-306, the error message could be more helpful by including the actual configured path to help with debugging.

4. Configuration Validation: Missing Directory Existence Check

The validation checks if the path is inside the public directory but doesn't verify if the configured directory actually exists. Consider adding a warning or validation for non-existent directories.

5. Documentation: Missing Migration Guide

While the PR description includes a configuration example, there's no documentation on how existing applications should migrate to this new security model.

6. Test Coverage: Missing Edge Cases

The tests miss several important scenarios:

• Handling of symlinks in paths
• Unicode characters in bundle names
• Relative vs absolute path handling
• Concurrent access scenarios

🚨 Security Considerations

1. Path Validation: The most critical issue is the lack of path traversal protection
2. Symlink Handling: Consider how symlinks should be treated in both configuration and runtime
3. File Permissions: No verification that the private directory has appropriate permissions

📝 Style & Convention Compliance

Per CLAUDE.md requirements:

• ✅ Code appears to follow RuboCop conventions
• ✅ Files seem to have trailing newlines
• ✅ No manual formatting detected

💡 Suggestions for Improvement

1. Add a validate_bundle_path helper method to centralize path security checks
2. Add logging when falling back from private to public paths (if allowed)
3. Consider making enforce_private_server_bundles default to true in future major version
4. Add a configuration validator that runs on app boot to catch misconfigurations early

📊 Test Coverage Assessment

The added tests are good but could be more comprehensive:

• ✅ Configuration validation tests
• ✅ Default value tests
• ❌ Integration tests with actual file operations
• ❌ Error recovery scenarios
• ❌ Performance impact tests

Conclusion

This is a valuable security enhancement that addresses an important concern. However, the path traversal vulnerability must be fixed before merging. Once that critical issue and the other security considerations are addressed, this will be a solid addition to the framework.

Verdict: Request changes - primarily due to the path traversal security issue.

Closing in favor of updated original PR #1798 which now has the same clean implementation after squashing.

Code Review: Server Bundle Security Features 🔍

I have reviewed this PR thoroughly and here is my feedback:

✅ Strengths

1. Security Enhancement

• Excellent addition of security features to prevent server-side rendering code exposure
• The enforce_private_server_bundles option provides good control over security enforcement
• Proper validation prevents misconfiguration (e.g., placing bundles in public directories when enforcement is enabled)

2. Backward Compatibility

• All defaults preserve existing behavior, allowing gradual adoption
• The implementation is non-breaking for existing users

3. Code Quality

• Clean, well-structured implementation following Ruby best practices
• Good use of private class methods for internal utilities
• Follows the existing patterns in the codebase

🎯 Areas for Improvement

1. Bundle Path Resolution Logic
In lib/react_on_rails/utils.rb, the bundle_js_file_path method skips manifest lookup entirely when server_bundle_output_path is configured:

if server_bundle?(bundle_name) && config.server_bundle_output_path.present?
  return File.expand_path(File.join(root_path, config.server_bundle_output_path, bundle_name))
end

Concern: This bypasses webpack manifest entirely, which could cause issues when:

• Using hashed filenames in production
• The manifest contains the actual built filename that differs from the bundle name

Suggestion: Consider checking the manifest first, then falling back to the configured path if not found.

2. Error Messages Could Be More Helpful
The error in handle_missing_manifest_entry could provide more actionable guidance:

raise Shakapacker::Manifest::MissingEntryError,
      "Server bundle '#{bundle_name}' not found in manifest and " \
      "enforce_private_server_bundles is enabled. " \
      "Ensure server bundle is built and server_bundle_output_path is configured correctly."

Suggestion: Add the actual configured path to help debugging:

"Server bundle '#{bundle_name}' not found in manifest. " \
"Expected location: #{config.server_bundle_output_path}. " \
"Ensure the bundle is built to this location."

3. Method Naming Consistency

• public_bundles_full_path vs generated_assets_full_path - the deprecation is good, but consider using a clearer deprecation warning

📋 Test Coverage

The test coverage is comprehensive and well-structured:

• ✅ Configuration validation tests
• ✅ Path resolution with various scenarios
• ✅ Enforcement behavior tests
• ✅ Backward compatibility tests

Minor suggestion: Consider adding an integration test that actually builds a server bundle to a private location and verifies it is loaded correctly.

🔒 Security Considerations

The security implementation is solid:

• ✅ Validates paths are outside public directory
• ✅ No fallback to public paths when enforcement is enabled
• ✅ Clear configuration requirements

One additional consideration: Document that users should also configure their web server (nginx/apache) to explicitly block access to server bundle patterns, as defense in depth.

🚀 Performance

No performance concerns identified. The path resolution is efficient and properly memoized where needed.

📝 Documentation

Consider adding:

1. Migration guide for existing users who want to adopt private server bundles
2. Web server configuration examples (nginx/apache) to block server bundle patterns
3. Troubleshooting section for common issues (e.g., manifest not found errors)

✨ Overall Assessment

This is a well-implemented security feature that addresses a real concern about SSR code exposure. The implementation is clean, maintains backward compatibility, and includes good test coverage. With the minor improvements suggested above (particularly around manifest handling), this will be a valuable addition to React on Rails.

Recommendation: Approve with minor suggestions ✅

The code is ready to merge after considering the bundle path resolution suggestion. The other items are nice-to-haves that could be addressed in follow-up PRs.

Great work on this security enhancement! 🎉

🤖 Generated with Claude Code